feat: e1, e3 implementation for sha2 + keccak extensions #1657
Conversation
…inor improvements
CodSpeed Instrumentation Performance ReportMerging #1657 will not alter performanceComparing Summary
|
CodSpeed Walltime Performance ReportMerging #1657 will not alter performanceComparing
|
|
@arayikhalatyan we shouldn't have any block size restrictions on E1 (but likely need it for E2 in some light form). My imagination is indeed that E1 should just look like |
Yeah I agree. Then we have to make some changes to the GuestMemory interface because it expects the block size as a const generic. |
arayikhalatyan
changed the title
There was a problem hiding this comment.
this was a lot cleaner than I thought!
| @@ -44,6 +44,28 @@ pub struct Sha256VmAir { | |||
| pub(super) padding_encoder: Encoder, | |||
| } | |||
|
|
|||
| impl Sha256VmAir { | |||
| pub fn new( | |||
There was a problem hiding this comment.
info: this was moved from Sha256VmChip constructor
| impl Sha256StepHelper { | ||
| pub fn new() -> Self { | ||
| Self { | ||
| row_idx_encoder: Encoder::new(18, 2, false), |
There was a problem hiding this comment.
did you have to hardcode 18 because Padding::COUNT is not accessible in this crate?
There was a problem hiding this comment.
maybe this function should be renamed and moved to tests.rs if it's only used there
There was a problem hiding this comment.
18 comes from NUM_ROWS+1, I think it makes sense to keep it inside the constructor but with constants. eventually should be in colsref after Avaneesh's PR
| let num_blocks = num_keccak_f(remaining_len); | ||
| let mut hasher = Keccak::v256(); | ||
|
|
||
| trace |
There was a problem hiding this comment.
note: I think the way this is done makes sense ATM, but we'll have to change it / re-think it pretty carefully for the e4 rewrite
| Arc::new(self.air) | ||
| } | ||
| impl<F: PrimeField32, CTX> TraceStep<F, CTX> for KeccakVmStep { | ||
| fn execute( |
There was a problem hiding this comment.
I'm assuming this is mostly copied from InstructionExecutor in the other file
Implemented e1, e3 for sha2 and keccak extensions. I feel like the trace generation of sha2 is more readable now. Also, implemented an arbitrary length read function (used in both e1 executions) I think eventually we should reimplement Plonky3's trace generation for Keccak so we don't have to allocate a separate trace matrix for the perm cols. TODOs: - port the tests of keccak to the new framework - spend some time (not much) on optimizing the keccak trace gen Resolves INT-3966 Resolves INT-3921
Implemented e1, e3 for sha2 and keccak extensions. I feel like the trace generation of sha2 is more readable now.
Also, implemented an arbitrary length read function (used in both e1 executions)
I think eventually we should reimplement Plonky3's trace generation for Keccak so we don't have to allocate a separate trace matrix for the perm cols.
TODOs:
Resolves INT-3966
Resolves INT-3921